Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Container: Clone + 'static #540

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antiguru
Copy link
Member

Remove the requirement that all Container implementations are Clone and 'static. This makes implementing types simpler that depend on Container, and requires us to explicitly mark various places as Data such that they comply with Timely's type requirements.

@frankmcsherry
Copy link
Member

I started to look at this, and one of the first complaints from rust about removing Clone is the number of RefOrMut::swap calls we have. These are potentially an antipattern with containers, in that we'll likely be expecting to read a bunch of references out rather than owned contents, and cloning the container (and contents) while close to mandatory for Ref variants normally, probably isn't what we want with containers.

Probably not a big deal at the moment in that I think most Ref variants are e.g. abomonation related, and that's a different antipattern. But we might want to have a think about the right way to encourage good behavior here, and whether we are even doing the right thing ourselves.

Remove the requirement that all Container implementations are Clone and
'static. This makes implementing types simpler that depend on Container,
and requires us to explicitly mark various places as `Data` such that they
comply with Timely's type requirements.

Signed-off-by: Moritz Hoffmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants